-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Experiment] Patterns: Allow the heading block and the button block to be partially synced #56704
[Experiment] Patterns: Allow the heading block and the button block to be partially synced #56704
Conversation
This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ lib/experimental/blocks.php |
fce4394
to
5924426
Compare
Size Change: -13.5 kB (-1%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for exploring this!
I left some comments. It seems like we need to strip this back a bit until the HTML tag processor has more functionality, but heading support is definitely useful 👍
'tag_name' => $block_type->attributes[ $attribute_name ]['selector'], | ||
) | ||
); | ||
$selectors = explode( ',', $block_type->attributes[ $attribute_name ]['selector'] ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be good to add a comment above this saying something like:
// Support comma-separated selectors by exploding them into an array.
I guess it's obvious in the context of this PR given it mentions the various heading block tags, but it might not be so obvious if you're just looking at this file on its own wondering why it works this way.
$selectors = explode( ',', $block_type->attributes[ $attribute_name ]['selector'] ); | ||
$found = false; | ||
while ( ! $found && count( $selectors ) > 0 ) { | ||
$tags = new WP_HTML_Tag_Processor( $block_content ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, it might be worth a comment saying something like:
// Creating a new tag processor on every loop resets the search to the start.
// - Heading: content. | ||
// - Button: text. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really sure these comments are necessary, seeing how the same thing is listed below in an easy to read format. 😄
// - Image: url. | ||
$blocks_attributes_allowlist = array( | ||
'core/paragraph' => array( 'content' ), | ||
'core/heading' => array( 'content' ), | ||
'core/button' => array( 'text' ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, button blocks don't seem to be working correctly. When they have an override they're missing the <div class="wp-block-button">
wrapper.
It's because the code below does this:
$tag_name = $tags->get_tag();
$markup = "<$tag_name>$custom_value</$tag_name>";
$updated_tags = new WP_HTML_Tag_Processor( $markup );
$updated_tags->next_tag();
return $updated_tags->get_updated_html();
So it considers the updated html of the block to just be the tag that it's updating and disregards any markup before or after that.
Instead it needs to update the $original_tags
and get_updated_html
from that, but I don't think it's supported yet 🤔
It is mentioned in the block bindings tracking issue - #54536.
Support to mutate HTML to change the inner content of an element. Until we have a set_inner_markup function, we can manually mutate the HTML for our experiments.
I guess we need to drop button support until then.
Image also doesn't work, as the code only updates tag content, not attribute values, so you end up with <image>$url</image>
and it would also strip the image block <figure>
wrapper, so I think that should be removed too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SantosGuillamot opened #57249 that updates the PHP implementation for block bindings that should fix the handling for the Heading and Button blocks. It was extracted from the full prototype for Custom Fields: #56867.
Ah, I see there's a proposed updated version of the code in #56867 |
Co-authored-by: Daniel Richards <daniel.richards@automattic.com>
// TODO: In the future, to support connecting more block attributes, | ||
// we should build the full query from CSS selector. | ||
$found = $tags->next_tag( trim( array_shift( $selectors ) ) ); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could invert this loop and avoid the confusion @talldan wants to clarify, while improving performance.
- iterate through all the tags until
- we find a tag matching one of the selectors
this assumes that there are likely more tags than selectors, many more, and so the cost of checking each selector at each tag will be cheaper than checking every tag at each selector. additionally, we should make it pretty explicit here in a comment that the only selectors we currently support are tag names.
$selectors = array();
foreach ( explode( ',', $block_type->attributes[ $attribute_name ]['selector'] ) as $tag_name ) {
$selectors[] = trim( strtoupper( $tag_name ) );
}
$tags = new WP_HTML_Tag_Processor( $block_content );
while ( $tags->next_tag() ) {
$tag_name = $tags->get_tag();
if ( ! in_array( $tag_name, $selectors, true ) ) {
continue;
}
$markup = "<$tag_name>$custom_value</$tag_name>";
$updated_tags = new WP_HTML_Tag_Processor( $markup );
$updated_tags->next_tag();
// Get all the attributes from the original block and add them to the new markup.
$names = $tags->get_attribute_names_with_prefix( '' );
foreach ( $names as $name ) {
$updated_tags->set_attribute( $name, $tags->get_attribute( $name ) );
}
return $updated_tags->get_updated_html();
}
return $block_content;
This should be superseded by #57249. |
What?
Based on #56235. Allow the
core/heading
block and thecore/button
block to be partially synced.Why?
To have more blocks to test the feature.
How?
I had to refactor a part of the block connections code to support
,
-separated attributes' selectors. It'd be nice to get a sanity check that it doesn't break or block the work on the block connections side. (c.c. @SantosGuillamot)Note that the solution here is likely just a temporary solution. Ideally, we might want to support arbitrary CSS selectors in the
WP_HTML_Tag_Processor
API.Testing Instructions
Screenshots or screencast
Kapture.2023-12-01.at.11.56.36.mp4